Skip to content

WIP: Submitter Assemble Batch#864

Open
Kukoomomo wants to merge 20 commits intomainfrom
feat/submitter_batch
Open

WIP: Submitter Assemble Batch#864
Kukoomomo wants to merge 20 commits intomainfrom
feat/submitter_batch

Conversation

@Kukoomomo
Copy link
Contributor

@Kukoomomo Kukoomomo commented Feb 3, 2026

Summary by CodeRabbit

  • New Features

    • Full batch lifecycle: assembly, sealing, persistent storage and retrieval of rollup batches
    • Blob-capable L1 transaction support and expanded transaction decoding
    • Multi-node L2 client support and a new L2 caller for sequencer/gov queries
    • New rollupDelayPeriod config (default 86400) with non-zero validation
  • Improvements

    • Stronger batch integrity and on-chain consistency checks
    • Automatic fallback for failed resubmissions (replacement transfer)
    • LastCacheBatchIndex metric added; default batch interval increased to 5s

@Kukoomomo Kukoomomo requested a review from a team as a code owner February 3, 2026 15:14
@Kukoomomo Kukoomomo requested review from secmgt and removed request for a team February 3, 2026 15:14
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 3, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.
📝 Walkthrough

Walkthrough

Adds a new batch subsystem (batch cache, data, headers, blob handling, storage), multi-client L2 caller, DB/metrics APIs, rollup init calls to initialize2/initialize3 using rollupDelayPeriod, and rewires Rollup service to use the new batch package while removing the legacy BatchFetcher/types.BatchCache.

Changes

Cohort / File(s) Summary
Batching Core
tx-submitter/batch/batch_cache.go, tx-submitter/batch/batch_data.go, tx-submitter/batch/batch_header.go
Implements a full BatchCache lifecycle: init/sync from rollup/DB, block accumulation, capacity/timeout checks, sealing (V0/V1), hashing, storage integration and public APIs for assemble/seal/query.
Blob & Tx I/O
tx-submitter/batch/blob.go, tx-submitter/batch/commit_test.go
Adds blob canonicalization/commitment, blob tx sidecar creation, zstd compression, and robust DecodeTxsFromBytes; includes a commit test exercising blob submission and fee logic.
Storage & Persistence
tx-submitter/batch/batch_storage.go, tx-submitter/batch/batch_storage_test.go, tx-submitter/db/db.go, tx-submitter/db/interface.go, tx-submitter/mock/db.go
Adds BatchStorage (LevelDB JSON persistence, indices), byte-oriented Db APIs and interface methods, and mock DB byte methods for tests.
Batch Query & Restart Tests
tx-submitter/batch/batch_query.go, tx-submitter/batch/batch_restart_test.go, tx-submitter/batch/batch_cache_test.go
Implements on-chain Finalize/Commit event scanning and parsing, commit parsing helpers, assembly helpers, and tests for restart/assembly and BatchCache behaviors.
Types & Multi-client L2 Caller
tx-submitter/iface/client.go, tx-submitter/types/l2Caller.go, tx-submitter/iface/rollup.go, tx-submitter/types/converter.go
Adds L2Clients wrapper for multi-client resilience, L2Caller for predeploy contract calls, new IRollup/IL2Gov methods and IL2MessagePasser, and helper converters for endianness/height parsing.
Service Integration
tx-submitter/services/rollup.go, tx-submitter/entry.go, tx-submitter/services/rollup_handle_test.go
Rollup constructor now accepts L2Caller; replaces legacy types.BatchCache/BatchFetcher with new batch.BatchCache; starts background sync/assembly loops; updates submission/finalize flows and adds replacement-transfer txn logic.
Removed / Replaced
tx-submitter/services/batch_fetcher.go, tx-submitter/types/batch_cache.go, tx-submitter/types/*_test.go
Removes legacy BatchFetcher and old types.BatchCache and their tests; replaced by new batch package implementation and tests.
Metrics, Flags & Misc
tx-submitter/metrics/metrics.go, tx-submitter/flags/flags.go, tx-submitter/.gitignore, tx-submitter/utils/methods.go
Adds LastCacheBatchIndex metric and setter, increases RollupInterval default (500ms → 5s), adds submitter-leveldb ignore, and a comment localization.
Contracts & Deploy
contracts/deploy/020-ContractInit.ts, contracts/src/deploy-config/l1.ts
Adds rollupDelayPeriod to config with non-zero guard; calls Rollup.initialize2(...) then Rollup.initialize3(rollupDelayPeriod) in deploy flow; updates genesis batchHeader hex.

Sequence Diagram(s)

sequenceDiagram
    participant RC as Rollup Service
    participant BC as BatchCache
    participant L2 as L2 Client(s)
    participant L1 as L1 / Rollup Contract
    participant BS as BatchStorage/DB

    RC->>BC: Start background sync (Init / InitAndSyncFromDatabase)
    activate BC
    BC->>L1: Query batch status / last finalized index
    BC->>L2: Fetch blocks & headers for range
    L2-->>BC: Return headers & tx payloads
    BC->>BC: CalculateCapWithProposalBlock / PackCurrentBlock
    alt capacity or timeout reached
        BC->>BC: SealBatch (compress, compute data hash, build header)
        BC->>BS: StoreSealedBatch
        BS-->>BC: Ack
        BC->>RC: Expose batch for submission
    end
    BC-->>RC: Get / LatestBatchIndex used in submission flow
    deactivate BC
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

Suggested reviewers

  • FletcherMan
  • secmgt

Poem

🐇
I hop through headers, bytes in tow,
I stitch the blobs where soft winds blow,
Cache the batches, seal them tight,
Commit at dawn and hum all night,
A rabbit's cheer — the rollup grows!

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.44% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'WIP: Submitter Assemble Batch' is a work-in-progress indicator with a general description of the feature area, but lacks specificity about the primary changes. Clarify the title to reflect the main objective—e.g., 'Add BatchCache implementation for batch assembly' or 'Implement batch data accumulation and sealing logic'—to better communicate the changeset's primary purpose to reviewers scanning history.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/submitter_batch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tx-submitter/services/rollup.go (1)

1210-1211: ⚠️ Potential issue | 🟠 Major

Potential precision loss with Int64() truncation.

Same issue as in commit_test.go—converting *big.Int to int64 can truncate large gas fee values during network congestion.

🔧 Proposed fix
 return ethtypes.NewTx(&ethtypes.BlobTx{
 	ChainID:    uint256.MustFromBig(r.chainId),
 	Nonce:      nonce,
-	GasTipCap:  uint256.MustFromBig(big.NewInt(tip.Int64())),
-	GasFeeCap:  uint256.MustFromBig(big.NewInt(gasFeeCap.Int64())),
+	GasTipCap:  uint256.MustFromBig(tip),
+	GasFeeCap:  uint256.MustFromBig(gasFeeCap),
 	Gas:        gas,
🤖 Fix all issues with AI agents
In `@contracts/deploy/020-ContractInit.ts`:
- Around line 72-76: The initialize2 call is not awaited and its transaction
result isn't checked; make Rollup.initialize2(...) use await and capture/wait
for its receipt (e.g., store its result and call .wait()) before calling
Rollup.initialize3, and update the console.log for initialize3 to include the
actual parameter value passed (use the variable or literal passed to initialize3
in the log) so the output shows the parameter and success/failure status;
reference Rollup.initialize2, Rollup.initialize3, res and rec when making these
changes.

In `@tx-submitter/batch/batch_cache_test.go`:
- Around line 27-34: Tests currently ignore dial errors (using blank
identifier), which can mask connection failures and produce nil clients; update
the test setup and any helper in common_test.go that dials L1/L2 to capture the
returned error instead of `_`, assert it is nil and fail the test (use t.Fatalf
or require.NoError) so NewBatchCache and methods like InitFromRollupByRange
never receive nil clients; ensure the dial call sites that produce l1Client and
l2Client return both (client, err) and propagate/assert the err immediately.

In `@tx-submitter/batch/batch_cache.go`:
- Around line 350-427: The check in CalculateCapWithProposalBlock allows
re-processing block 0 even after progress, which can regress state; replace the
current special-case "blockNumber != 0" logic with a stricter allowance that
only permits blockNumber==0 when bc.lastPackedBlockHeight==0, and otherwise
treat any blockNumber <= bc.lastPackedBlockHeight as an error; update the
initial continuity guard (the if using bc.lastPackedBlockHeight and blockNumber)
to implement this rule so calls with blockNumber==0 after progress are rejected.
- Around line 999-1092: AssembleCurrentBatchHeader currently initializes
startBlockNum to lastPackedBlockHeight which makes the batch start one block too
early; change startBlockNum to lastPackedBlockHeight+1 and fetch startBlock
using that value (update the initial startBlock/get startBlockTime logic), keep
the existing post-seal update to startBlockNum = blockNum+1 but ensure you only
call BlockByNumber with the corrected startBlockNum when recomputing
startBlockTime for the next batch; references: AssembleCurrentBatchHeader,
startBlockNum, lastPackedBlockHeight, PackCurrentBlock, SealBatch.
- Around line 841-921: The loop incorrectly sets startBlockNum = blockNum+1
after SealBatchAndCheck, causing the current block to be skipped or
interval/timeout accounting to shift; instead set startBlockNum = blockNum so
the current block becomes the first block of the new batch and set
startBlockTime = nowBlockTime (use the already fetched nowBlock) rather than
fetching startBlock for blockNum+1; update
assembleUnFinalizeBatchHeaderFromL2Blocks to assign startBlockNum = blockNum and
startBlockTime = nowBlockTime after successful sealing (references:
startBlockNum, startBlockTime, nowBlockTime, SealBatchAndCheck,
PackCurrentBlock).

In `@tx-submitter/batch/batch_data.go`:
- Around line 129-139: Rename the misleading function
EstimateCompressedSizeWithNewPayload to a clear predicate name such as
WillExceedSizeLimit (or similar) and update call sites accordingly; prevent
accidental mutation of cks.txsPayload by building the concatenated blob from a
new slice (e.g., allocate a fresh slice and copy cks.txsPayload and txPayload
into it or append onto a nil slice) before checking len against MaxBlobBytesSize
and before calling zstd.CompressBatchBytes; keep the original boolean-returning
semantics (whether the compressed data would exceed MaxBlobBytesSize) and
preserve error propagation from zstd.CompressBatchBytes.

In `@tx-submitter/batch/batch_query.go`:
- Around line 280-294: The direct type assertion args[0].(struct{...}) is unsafe
and can panic; replace it with an error-checked assertion or type switch to
verify the concrete type before using it (e.g., check args[0] is the expected
struct type and return an error if not). Also remove the manual loop converting
ParentBatchHeader from []uint8 to []byte—treat
batchDataInputStruct.ParentBatchHeader as []byte directly (they are identical in
Go) when assigning to parentBatchHeader; update usages of batchDataInputStruct,
args and ParentBatchHeader accordingly so the code handles ABI mismatches safely
and avoids the unnecessary conversion.

In `@tx-submitter/batch/batch_restart_test.go`:
- Around line 493-496: getInfosFromContract currently discards errors from
rollupContract.LastCommittedBatchIndex and
rollupContract.LastFinalizedBatchIndex; update it to capture both error returns
(from LastCommittedBatchIndex and LastFinalizedBatchIndex), validate them
(ensure neither is non-nil), and propagate or fail instead of returning
potentially invalid indices—e.g., change getInfosFromContract to return
(*big.Int, *big.Int, error) and return an error if either call fails, or if this
helper is only used in tests call t.Fatalf with the captured error; reference
the function name getInfosFromContract and the methods
rollupContract.LastCommittedBatchIndex and
rollupContract.LastFinalizedBatchIndex when making the change.

In `@tx-submitter/batch/blob.go`:
- Around line 44-53: In RetrieveBlobBytes, the error message references the
wrong variable (uses data[i*32]) when checking high-order bytes; change the
error construction to report blob[i*32] instead of data[i*32] so the logged
value reflects the actual source byte being validated (update the fmt.Errorf
call inside the loop that currently checks blob[i*32] and reports data[i*32]);
ensure this uses the blob slice and keeps the same message format.
- Around line 189-198: In extractInnerTxFullBytes detect and reject malformed
RLP lengths by validating sizeByteLen (computed from firstByte - 0xf7) before
allocating/reading: ensure sizeByteLen is between 1 and 4 inclusive, return a
clear error if outside that range (e.g., "invalid RLP length bytes"), and only
then read sizeByteLen into sizeByte and compute size; this prevents the
negative-length/panic scenario when firstByte > 0xfb and guards against
empty/zero/oversized length encodings.
- Around line 18-22: The globals emptyBlob, emptyBlobCommit and emptyBlobProof
are initialized while ignoring errors from kzg4844.BlobToCommitment and
kzg4844.ComputeBlobProof; move their initialization into an init() function (or
package init block) where you call kzg4844.BlobToCommitment(emptyBlob) and
kzg4844.ComputeBlobProof(emptyBlob, emptyBlobCommit), check returned errors, and
fail fast (log and panic/exit) if the trusted setup or proof computation errors
so invalid empty proofs are never used; reference the symbols emptyBlob,
emptyBlobCommit, emptyBlobProof and the functions BlobToCommitment and
ComputeBlobProof when making the change.

In `@tx-submitter/batch/commit_test.go`:
- Around line 127-128: The code truncates big.Ints by calling tip.Int64() and
gasFeeCap.Int64() before wrapping with uint256.MustFromBig, risking precision
loss; instead pass the original *big.Int values directly into
uint256.MustFromBig (e.g., replace the big.NewInt(tip.Int64()) pattern) and
ensure tip and gasFeeCap are non-nil/validated before conversion so
full-precision gas tip and fee cap are preserved.
- Around line 214-227: The fee calculation in the test (the local variable fee
computed from tx.BlobGasFeeCap().Uint64(), tx.BlobGas(), tx.GasPrice().Uint64(),
tx.Gas()) can overflow uint64 when multiplying large values; replace the direct
uint64 multiplies with safe checks (either use math/bits.Mul64 overflow
detection or check b > math.MaxUint64/a before multiplication) or perform
arithmetic with big.Int and then compare to txFeeLimit; ensure both blobFee and
txFee are computed with overflow-safe logic and the final comparison against
txFeeLimit uses the same safe numeric type.
- Around line 28-36: The test currently slices pk[2:] inside TestRollupWithProof
which will panic because pk is an empty string; fix by validating and preparing
the private key before slicing: in TestRollupWithProof, assert the test private
key is present (e.g., require.NotEmpty(t, pk) or read from a test env), ensure
it has the "0x" prefix (or add it) and only then call crypto.HexToECDSA on the
hex portion, or alternatively replace the placeholder pk with a known test
private key literal for the test; update references to pk, crypto.HexToECDSA,
and TestRollupWithProof accordingly.

In `@tx-submitter/iface/client.go`:
- Around line 248-257: The L2Clients.GetRollupBatchByIndex wrapper currently
returns the first client response even if it's nil or missing signatures; update
it to mirror the validation in the standalone rollup GetRollupBatchByIndex by
checking that the returned *eth.RPCRollupBatch is non-nil and contains
signatures (e.g., result != nil && len(result.Signatures) > 0) inside the
tryAllClients callback, skip/continue to the next client when the batch is
invalid, and only accept/return a batch that passes validation (otherwise
propagate the final error); modify the logic around tryAllClients, result, and
err accordingly so behavior matches the standalone implementation.

In `@tx-submitter/services/rollup.go`:
- Around line 257-270: The background goroutine should respect r.ctx
cancellation, use the existing utils.Loop pattern and implement
exponential/backoff on errors; replace the raw for loop with a utils.Loop(r.ctx,
...) call (or equivalent) that calls r.batchCache.InitAndSyncFromRollup() and
r.batchCache.AssembleCurrentBatchHeader(), logs errors including the error
value, and on failure waits with increasing/backoff delays (reset on success)
before retrying instead of immediately sleeping 5s, ensuring the loop returns
when r.ctx is done.

In `@tx-submitter/types/l2Caller.go`:
- Around line 74-87: In GetSequencerSetBytes update the error message string in
the final fmt.Errorf call to remove the double space ("verify  failed" → "verify
failed") so the message reads e.g. "sequencer set hash verify failed ...";
locate this in the L2Caller.GetSequencerSetBytes function (references:
sequencerContract.SequencerSetVerifyHash and
sequencerContract.GetSequencerSetBytes) and change only the error text.
🧹 Nitpick comments (15)
contracts/deploy/020-ContractInit.ts (1)

73-74: Consider extracting magic numbers to configuration.

The hardcoded values 0x...0001 and 8640000000 should ideally be sourced from the config object (similar to batchHeader, submitter, challenger) for maintainability and environment flexibility.

tx-submitter/entry.go (1)

210-213: Consider adding context to the error.

When NewL2Caller fails, the error is returned without additional context, making it harder to diagnose issues during startup.

💡 Suggested improvement
 		l2Caller, err := types.NewL2Caller(l2Clients)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to create L2 caller: %w", err)
 		}
tx-submitter/batch/commit_test.go (2)

77-83: Replace time.Sleep with retry-based receipt polling.

Using time.Sleep(2 * time.Second) is flaky—transaction confirmation time varies. If the transaction takes longer, the test fails; if faster, time is wasted.

♻️ Suggested approach
// Poll for receipt with timeout instead of fixed sleep
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
defer cancel()

var receipt *ethtypes.Receipt
for {
    receipt, err = l1Client.TransactionReceipt(ctx, transaction.Hash())
    if err == nil {
        break
    }
    select {
    case <-ctx.Done():
        t.Fatal("timeout waiting for receipt")
    case <-time.After(500 * time.Millisecond):
        // retry
    }
}

138-150: estimateGas function is unused.

This helper function is defined but never called in the test file. Consider removing dead code or using it appropriately.

tx-submitter/batch/batch_cache_test.go (1)

30-51: Test design is unsuitable for CI/automated testing.

TestBatchCacheInitServer runs an infinite loop and waits for os.Interrupt (CTRL-C), which will cause CI pipelines to hang or timeout. This appears to be a manual debugging/integration test.

Consider:

  1. Adding a build tag to exclude from regular test runs (e.g., //go:build integration)
  2. Adding a timeout context
  3. Renaming to indicate it's not a unit test
♻️ Suggested approach
+//go:build integration
+
 package batch
 
 ...
 
-func TestBatchCacheInitServer(t *testing.T) {
+func TestBatchCacheInitServer_Manual(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test in short mode")
+	}
+	
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+	
 	cache := NewBatchCache(nil, l1Client, []iface.L2Client{l2Client}, rollupContract, l2Caller)
 
-	go utils.Loop(cache.ctx, 5*time.Second, func() {
+	go utils.Loop(ctx, 5*time.Second, func() {
tx-submitter/services/rollup.go (1)

780-786: Redundant block number fetch and confirmations recalculation.

currentBlock is fetched at Line 737 and confirmations calculated at Line 742. Lines 782-786 re-fetch and recalculate the same values unnecessarily for successful transactions. This adds latency and RPC calls without benefit since only a few lines of code execute between the two fetches.

♻️ Suggested simplification
 	} else { // Transaction succeeded
 		// Get current block number for confirmation count only for successful transactions
-		currentBlock, err = r.L1Client.BlockNumber(context.Background())
-		if err != nil {
-			return fmt.Errorf("get current block number error: %w", err)
-		}
-		confirmations = currentBlock - status.receipt.BlockNumber.Uint64()
+		// currentBlock and confirmations already computed above at lines 737, 742
 
 		if method == constants.MethodCommitBatch {
tx-submitter/batch/blob.go (1)

168-176: Remove commented-out code.

This block of commented-out code appears to be superseded by the switch statement above. Dead code reduces readability and should be removed.

🧹 Remove dead code
-	// we support the tx types of LegacyTxType/AccessListTxType/DynamicFeeTxType
-	//if firstByte == eth.AccessListTxType || firstByte == eth.DynamicFeeTxType {
-	//	// the firstByte here is used to indicate tx type, so skip it
-	//	if err := binary.Read(reader, binary.BigEndian, &firstByte); err != nil {
-	//		return nil, err
-	//	}
-	//} else if firstByte <= 0xf7 { // legacy tx first byte must be greater than 0xf7(247)
-	//	return nil, fmt.Errorf("not supported tx type: %d", firstByte)
-	//}
 	fullTxBytes, err = extractInnerTxFullBytes(firstByte, reader)
tx-submitter/batch/batch_data.go (2)

73-84: Magic numbers for block context sizes should be constants.

The values 60 (block context size) and 58 (truncated context for hash) appear without explanation. Define named constants to improve maintainability and document the structure.

♻️ Suggested improvement
+const (
+	BlockContextSize       = 60  // Size of a single block context in bytes
+	BlockContextHashSize   = 58  // Bytes used from block context for DataHash (excludes last 2 bytes)
+)
+
 func (cks *BatchData) DataHash() common.Hash {
 	if cks.hash != nil {
 		return *cks.hash
 	}
 
 	var bz []byte
 	for i := 0; i < int(cks.blockNum); i++ {
-		bz = append(bz, cks.blockContexts[i*60:i*60+58]...)
+		bz = append(bz, cks.blockContexts[i*BlockContextSize:i*BlockContextSize+BlockContextHashSize]...)
 	}
 	bz = append(bz, cks.l1TxHashes...)
 	return crypto.Keccak256Hash(bz)
 }

88-106: Inconsistent hash caching between DataHash and DataHashV2.

DataHash() caches the result in cks.hash, but DataHashV2() does not. If DataHashV2 is called frequently, consider adding similar caching for consistency and performance.

tx-submitter/batch/batch_restart_test.go (3)

27-50: Duplicate global variables and init() across test files.

These global variables (rollupAddr, l1Client, l2Client, rollupContract, l2Caller) and the init() function are duplicated in batch_cache_test.go. Extract them to a shared common_test.go file to avoid maintenance issues.


632-637: Empty if block for exceeded capacity.

The exceeded flag is checked but the block is empty with only a comment. Either implement the early-stop logic or remove the dead code.

🧹 Suggested cleanup
-		// If capacity exceeds limit, can stop early (optional)
-		if exceeded {
-			// Note: Can choose to continue packing until endBlockNum, or stop early
-			// Decide based on business requirements
-		}
+		// If capacity exceeds limit, stop packing
+		if exceeded {
+			break
+		}

Or remove entirely if not needed:

-		// If capacity exceeds limit, can stop early (optional)
-		if exceeded {
-			// Note: Can choose to continue packing until endBlockNum, or stop early
-			// Decide based on business requirements
-		}

397-402: Replace custom min function with Go 1.21+ builtin.

The project targets Go 1.24.0, which includes the builtin min function. This custom implementation at lines 397-402 can be removed and replaced with min(a, b).

tx-submitter/batch/batch_query.go (2)

332-401: Duplicate struct conversion logic.

parseCommitBatchWithProofTxData duplicates the struct definition and conversion code from parseCommitBatchTxData. Extract common conversion helpers to reduce duplication.

♻️ Suggested extraction
// extractBatchDataInput extracts IRollupBatchDataInput from ABI-unpacked args
func extractBatchDataInput(arg interface{}) (*bindings.IRollupBatchDataInput, error) {
    s, ok := arg.(struct {
        Version           uint8     `json:"version"`
        ParentBatchHeader []uint8   `json:"parentBatchHeader"`
        // ... rest of fields
    })
    if !ok {
        return nil, fmt.Errorf("unexpected type: %T", arg)
    }
    return &bindings.IRollupBatchDataInput{
        Version:           s.Version,
        ParentBatchHeader: []byte(s.ParentBatchHeader),
        // ...
    }, nil
}

// Similar for extractBatchSignatureInput

46-54: Silent error handling on filter failure.

When FilterFinalizeBatch fails, the code continues querying backwards without logging. This could make debugging difficult if there's a persistent issue (e.g., RPC rate limiting).

🔧 Add logging
 		finalizeEventIter, err := bc.rollupContract.FilterFinalizeBatch(filterOpts, []*big.Int{new(big.Int).SetUint64(index)}, nil)
 		if err != nil {
+			log.Debug("filter finalize batch failed, continuing backwards", "error", err, "startBlock", startBlock, "endBlock", endBlock)
 			// If query fails, continue querying backwards
 			if endBlock < blockRange {
 				break // Already queried to block 0, exit loop
 			}
tx-submitter/batch/batch_header.go (1)

167-185: EncodedBytes cache never persists with value receivers.

Both Bytes() methods assign to EncodedBytes on a value receiver, so the cache is discarded after the call. Switch to pointer receivers (or remove caching) to avoid repeated allocations.

🔧 Suggested fix
-func (b BatchHeaderV0) Bytes() BatchHeaderBytes {
+func (b *BatchHeaderV0) Bytes() BatchHeaderBytes {
 	if len(b.EncodedBytes) > 0 {
 		return BatchHeaderBytes(b.EncodedBytes)
 	}
 	batchBytes := make([]byte, expectedLengthV0)
 	batchBytes[0] = BatchHeaderVersion0
 	binary.BigEndian.PutUint64(batchBytes[1:], b.BatchIndex)
 	binary.BigEndian.PutUint64(batchBytes[9:], b.L1MessagePopped)
 	binary.BigEndian.PutUint64(batchBytes[17:], b.TotalL1MessagePopped)
 	copy(batchBytes[25:], b.DataHash[:])
 	copy(batchBytes[57:], b.BlobVersionedHash[:])
 	copy(batchBytes[89:], b.PrevStateRoot[:])
 	copy(batchBytes[121:], b.PostStateRoot[:])
 	copy(batchBytes[153:], b.WithdrawalRoot[:])
 	copy(batchBytes[185:], b.SequencerSetVerifyHash[:])
 	copy(batchBytes[217:], b.ParentBatchHash[:])
 	b.EncodedBytes = batchBytes
 	return batchBytes
 }
@@
-func (b BatchHeaderV1) Bytes() BatchHeaderBytes {
+func (b *BatchHeaderV1) Bytes() BatchHeaderBytes {
 	if len(b.EncodedBytes) > 0 {
 		return BatchHeaderBytes(b.EncodedBytes)
 	}
 	batchBytes := make([]byte, expectedLengthV1)
 	batchBytes[0] = BatchHeaderVersion1
 	binary.BigEndian.PutUint64(batchBytes[1:], b.BatchIndex)
 	binary.BigEndian.PutUint64(batchBytes[9:], b.L1MessagePopped)
 	binary.BigEndian.PutUint64(batchBytes[17:], b.TotalL1MessagePopped)
 	copy(batchBytes[25:], b.DataHash[:])
 	copy(batchBytes[57:], b.BlobVersionedHash[:])
 	copy(batchBytes[89:], b.PrevStateRoot[:])
 	copy(batchBytes[121:], b.PostStateRoot[:])
 	copy(batchBytes[153:], b.WithdrawalRoot[:])
 	copy(batchBytes[185:], b.SequencerSetVerifyHash[:])
 	copy(batchBytes[217:], b.ParentBatchHash[:])
 	binary.BigEndian.PutUint64(batchBytes[249:], b.LastBlockNumber)

 	b.EncodedBytes = batchBytes
 	return batchBytes
 }

Also applies to: 195-215

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
tx-submitter/metrics/metrics.go (1)

210-224: ⚠️ Potential issue | 🟡 Minor

Add unregister for the new gauge.
Without this, re-initializing metrics can leave the registry dirty.

🔧 Suggested fix
 func (m *Metrics) UnregisterMetrics() {
 	prometheus.Unregister(m.WalletBalance)
 	prometheus.Unregister(m.RpcErrors)
 	prometheus.Unregister(m.RollupCostSum)
 	prometheus.Unregister(m.FinalizeCostSum)
 	prometheus.Unregister(m.RollupCost)
 	prometheus.Unregister(m.FinalizeCost)
 	prometheus.Unregister(m.CollectedL1FeeSum)
 	prometheus.Unregister(m.IndexerBlockProcessed)
 	prometheus.Unregister(m.LastCommittedBatch)
 	prometheus.Unregister(m.LastFinalizedBatch)
+	prometheus.Unregister(m.LastCacheBatchIndex)
 	prometheus.Unregister(m.HasPendingFinalizeBatch)
 	prometheus.Unregister(m.reorgs)
 	prometheus.Unregister(m.confirmedTxs)
 }
tx-submitter/services/rollup.go (1)

839-972: ⚠️ Potential issue | 🟡 Minor

Return a concrete error when the next batch is missing in cache.

On cache miss, the returned error uses a stale err value (often nil), which obscures the root cause.

🛠️ Suggested fix
 	if !ok {
 		log.Error("get next batch by index error",
 			"batch_index", nextBatchIndex,
 		)
-		return fmt.Errorf("get next batch by index err:%v", err)
+		return fmt.Errorf("next batch %d not found in cache", nextBatchIndex)
 	}
🤖 Fix all issues with AI agents
In `@tx-submitter/batch/batch_cache.go`:
- Around line 197-237: The code assumes the parent is at
batches[uint64(len(batches)-1)] which is unsafe for a map keyed by batch index
and can select the wrong parent or panic; instead determine the actual parent
index by computing the maximum key in the batches map (or derive from the
committed range fi/ci) and use that index to build the parentHeader via
BatchHeaderBytes, then proceed to set bc.lastPackedBlockHeight,
bc.parentBatchHeader and bc.prevStateRoot; if the computed parent index is
missing or out of the committed range call bc.InitAndSyncFromRollup() as the
existing logic does. Ensure you update references to batches[...] and any
variable that assumed zero-based contiguous keys (e.g., parentHeader,
lastPackedBlockHeight, prevStateRoot) to use the found maxIndex (or fi/ci)
instead.

In `@tx-submitter/batch/batch_restart_test.go`:
- Around line 31-53: The test init() currently ignores errors from
ethclient.Dial which can leave l1Client or l2Client nil and cause panics later;
update the Dial calls to capture and check errors (use l1Client, err =
ethclient.Dial(l1ClientRpc) and l2Client, err = ethclient.Dial(l2ClientRpc)),
and if err != nil return a clear failure (panic or fail the test) before using
the clients; likewise keep the existing error checks around bindings.NewRollup
and types.NewL2Caller to ensure all constructors only run with valid clients.

In `@tx-submitter/batch/commit_test.go`:
- Around line 191-207: The code can pass a nil blobFee into uint256.MustFromBig
causing a panic when head.ExcessBlobGas is nil; update getGasTipAndCap (and/or
createBlobTx) to guard blobFee before converting: ensure when head.ExcessBlobGas
== nil you set blobFee to zero (e.g., big.NewInt(0)) or return a safe zero
uint256 value, or add an explicit nil-check and branch so uint256.MustFromBig is
never called with nil; modify the places that construct the BlobTx to accept the
safe value (references: blobFee, head.ExcessBlobGas, getGasTipAndCap,
createBlobTx, uint256.MustFromBig, BlobTx constructor).

In `@tx-submitter/services/rollup.go`:
- Around line 257-275: The loop currently continues to call LatestBatchIndex()
and r.metrics.SetLastCacheBatchIndex(index) even when InitAndSyncFromDatabase or
AssembleCurrentBatchHeader fail, which can cause a panic; modify the goroutine
in rollup.go to bail out on any error from r.batchCache.InitAndSyncFromDatabase
or r.batchCache.AssembleCurrentBatchHeader (i.e., return from the iteration
early) and only call r.batchCache.LatestBatchIndex() and
r.metrics.SetLastCacheBatchIndex(index) if those previous calls succeeded and
LatestBatchIndex() itself returned no error; use the existing batchCacheSyncMu,
the functions InitAndSyncFromDatabase, AssembleCurrentBatchHeader,
LatestBatchIndex, and r.metrics.SetLastCacheBatchIndex to locate and implement
the early returns and guarded metric update.
🧹 Nitpick comments (2)
tx-submitter/batch/commit_test.go (1)

59-61: Avoid stale err usage after cache.Get.
cache.Get doesn’t return an error; the require.NoError(t, err) is redundant and potentially misleading.

♻️ Suggested cleanup
 	batch, exist := cache.Get(latestCommitBatchIndex.Uint64() + 1)
-	require.NoError(t, err)
 	require.True(t, exist)
tx-submitter/batch/batch_storage.go (1)

271-278: Consider surfacing DB errors in BatchExists.

Right now any DB error is treated as “not found,” which can mask underlying I/O failures. Consider returning (bool, error) or at least logging non-notfound errors.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tx-submitter/services/rollup.go (1)

706-742: ⚠️ Potential issue | 🟠 Major

Remove the redundant r.pendingTxs.Add(replacedTx) call — the transaction is already added by SendTx.

createReplacementTransferTx calls r.SendTx(newTx) (line 2020), which internally adds the transaction to pendingTxs at line 1554. Then handleDiscardedTx redundantly calls r.pendingTxs.Add(replacedTx) again at line 734, causing the replacement transaction to be added twice.

Either remove the Add call in handleDiscardedTx since SendTx already handles it, or refactor createReplacementTransferTx to return the signed transaction without calling SendTx, letting the caller manage the pending transaction state.

🤖 Fix all issues with AI agents
In `@tx-submitter/batch/batch_cache.go`:
- Around line 256-259: The code silently ignores the error returned by
parentHeader.TotalL1MessagePopped(), which can hide a corrupted parent header
and leave bc.totalL1MessagePopped at 0; update the block in the function that
sets bc.currentBlockNumber and bc.totalL1MessagePopped to capture the (value,
err) from parentHeader.TotalL1MessagePopped(), check err, and handle it (return
the error or wrap it with context) instead of discarding it so failures in
TotalL1MessagePopped are propagated; reference
parentHeader.TotalL1MessagePopped(), bc.totalL1MessagePopped, and
bc.currentBlockNumber when making the change.
- Around line 436-454: The continuity check in
BatchCache.CalculateCapWithProposalBlock is a TOCTOU because bc.mu is unlocked
before the expensive L2 client BlockByNumber call and re-locked later, allowing
lastPackedBlockHeight to change (e.g., via PackCurrentBlock); fix by moving the
BlockByNumber call outside the locked section but then re-acquiring bc.mu and
re-validating the same continuity conditions against lastPackedBlockHeight
before mutating state or returning; reference the method
CalculateCapWithProposalBlock, the mutex bc.mu, the state field
bc.lastPackedBlockHeight, and make sure any early returns (wrong/discontinuous
block number) and subsequent logic execute only after the re-check under lock.
- Around line 770-778: The code reads fields from bc.parentBatchHeader (calling
TotalL1MessagePopped(), BatchIndex(), and Hash()) but discards their errors,
which can silently produce zero values; update the block that initializes
parentBatchHeaderTotalL1, parentBatchBatchIndex, and parentBatchHash to capture
and check each error and return (or propagate) the first non-nil error instead
of ignoring it, and then update callers such as SealBatch to handle the new
error return; specifically modify the logic around bc.parentBatchHeader, change
the local assignments to receive (value, err) from
TotalL1MessagePopped/BatchIndex/Hash, check err and propagate it upward, and
adjust SealBatch (and any other callers) to accept and handle the error.

In `@tx-submitter/batch/batch_query.go`:
- Around line 260-303: The two parsing blocks in parseCommitBatchTxData and
parseCommitBatchWithProofTxData duplicate unsafe type assertions and conversion
logic; extract a shared helper (e.g., parseBatchDataAndSignature or similar)
that takes args []interface{} (or the two interface{} params) and returns
(*bindings.IRollupBatchDataInput, *bindings.IRollupBatchSignatureInput, error),
perform comma-ok type assertions when casting args[0] and args[1], convert
[]uint8 -> []byte and big.Int safely inside that helper, and replace both
parseCommitBatchTxData and parseCommitBatchWithProofTxData logic to call this
helper and handle the returned error; ensure the helper references the same
field names (Version, ParentBatchHeader, LastBlockNumber, NumL1Messages,
PrevStateRoot, PostStateRoot, WithdrawalRoot, SignedSequencersBitmap,
SequencerSets, Signature) so mapping is identical.

In `@tx-submitter/batch/batch_storage.go`:
- Around line 202-210: The comparison in updateBatchIndices currently uses a
direct equality check (err == db.ErrKeyNotFound) which fails for wrapped errors;
change that check to use errors.Is(err, db.ErrKeyNotFound) and ensure the errors
package is imported if not already, keeping the existing logic that initializes
indices to []uint64{} when the key is not found while returning other errors
from loadBatchIndices.
- Around line 160-190: StoreSealedBatches currently writes only the current
batch keys to saveBatchIndices, which overwrites and orphans previously stored
batches; change it to load the existing indices (via loadBatchIndices or
GetStoredBatchIndices), merge them with the newly appended indices (using
encodeBatchKey/idx to identify duplicates), deduplicate the combined list, and
then call saveBatchIndices with the merged list so older batches remain
reachable; alternatively, if replacement was intended, delete any old batch keys
not in the new indices before calling saveBatchIndices.

In `@tx-submitter/batch/commit_test.go`:
- Around line 58-60: The test incorrectly calls require.NoError(t, err) after
cache.Get even though cache.Get returns (*eth.RPCRollupBatch, bool) and does not
set err; remove the stale require.NoError(t, err) and instead assert the
returned batch is present (e.g. keep require.True(t, exist) and add
require.NotNil(t, batch) or equivalent) to make the intent explicit; update the
lines around the cache.Get call (referencing cache.Get and
latestCommitBatchIndex) accordingly.

In `@tx-submitter/services/rollup.go`:
- Around line 257-276: The closure passed to utils.Loop should use local error
variables and correct the log text: change assignments like `err =
r.batchCache.InitAndSyncFromDatabase()` and `err =
r.batchCache.AssembleCurrentBatchHeader()` to short declarations `err := ...` so
they don't clobber the outer Start() `err`, and update the log message that
currently reads "init and sync from rollup failed" to "init and sync from
database failed" to match the call to r.batchCache.InitAndSyncFromDatabase();
keep the other logging and the r.metrics.SetLastCacheBatchIndex(index) usage
intact.

In `@tx-submitter/types/l2Caller.go`:
- Line 12: The import for hexutil in tx-submitter/types/l2Caller.go currently
pulls from github.com/ethereum/go-ethereum/common/hexutil and must be changed to
the Morph fork; update the import to
github.com/morph-l2/go-ethereum/common/hexutil so it matches other files and
avoids mixed upstream forks—locate the import block in l2Caller.go and replace
the hexutil import path (references to hexutil functions/usages in that file
remain unchanged).
🧹 Nitpick comments (15)
contracts/deploy/020-ContractInit.ts (2)

60-71: Use strict equality and consider broader validation for rollupDelayPeriod.

Line 68 uses loose equality (==). With TypeScript, prefer === to avoid type coercion surprises (e.g., "0" == 0 is true). Also consider validating that the value is a positive integer, not just non-zero.

Proposed fix
-        if (rollupDelayPeriod==0){
-            console.error('rollupDelayPeriod cannot set zero')
+        if (!Number.isInteger(rollupDelayPeriod) || rollupDelayPeriod <= 0) {
+            console.error('rollupDelayPeriod must be a positive integer')
             return ''
         }

79-79: Hardcoded magic bytes32 value should be documented or extracted to a named constant.

"0x0000...0001" is passed to initialize2 without any explanation. A brief comment or a named constant (e.g., const INITIAL_L2_CALLER = ...) would help future readers understand the intent.

tx-submitter/batch/batch_query.go (1)

19-93: Silent error swallowing may mask persistent failures.

In getLastFinalizeBatchHeaderFromRollupByIndex, when FilterFinalizeBatch fails (line 47) or TransactionByHash/parseFinalizeBatchTxData fail (lines 64, 70), errors are silently skipped. Consider logging these at Warn level so persistent issues (e.g., RPC failures, ABI mismatches) are diagnosable in production.

tx-submitter/batch/batch_storage.go (2)

62-83: LoadSealedBatch wraps ErrKeyNotFound into a formatted error, preventing callers from distinguishing "not found" from I/O errors.

On line 71, the original sentinel is replaced with fmt.Errorf("sealed batch %d not found", ...). Callers cannot use errors.Is(err, db.ErrKeyNotFound). Consider using %w to preserve the sentinel or returning a dedicated not-found error.

Suggested fix
 		if errors.Is(err, db.ErrKeyNotFound) {
-			return nil, fmt.Errorf("sealed batch %d not found", batchIndex)
+			return nil, fmt.Errorf("sealed batch %d not found: %w", batchIndex, db.ErrKeyNotFound)
 		}

37-60: Index update failures are silently tolerated — consider the consistency implications.

Both StoreSealedBatch (line 55) and DeleteSealedBatch (line 127) log a warning but continue when updateBatchIndices fails. This means the batch data and the indices list can drift: a stored batch might be missing from the indices list, or a deleted batch might still be referenced. LoadAllSealedBatches handles the latter gracefully (line 104-107), but the former case means a batch can be stored yet invisible to LoadAllSealedBatches.

If this is an accepted trade-off, a short comment explaining why would help future maintainers.

Also applies to: 116-133

tx-submitter/types/l2Caller.go (1)

74-87: TOCTOU risk between SequencerSetVerifyHash and GetSequencerSetBytes.

The hash (line 75) and set bytes (line 79) are fetched in separate RPC calls. If opts doesn't pin a specific BlockNumber, the sequencer set could change between the two calls, causing a spurious verification failure. Callers should ensure opts.BlockNumber is set. A brief doc comment on this function noting the requirement would help.

tx-submitter/mock/rollup.go (1)

88-96: Nil iterator returns from FilterCommitBatch/FilterFinalizeBatch will panic on .Next() calls.

Both methods return nil, nil, but callers immediately invoke .Next() without nil-checking (e.g., batch_query.go:46, batch_restart_test.go:297/396, oracle/batch.go:226). Returning a zero-value iterator or adding setter methods would prevent nil pointer dereferences in tests that exercise event iteration.

tx-submitter/batch/batch_restart_test.go (3)

297-305: Filter errors are silently swallowed, making debugging difficult.

When FilterFinalizeBatch fails, the error is discarded and the loop simply retries a different range. At minimum, log the error for observability. The same pattern exists in getCommitBatchDataByIndex (Line 397–404).

🔧 Proposed fix
 		finalizeEventIter, err := rollupContract.FilterFinalizeBatch(filterOpts, []*big.Int{new(big.Int).SetUint64(index)}, nil)
 		if err != nil {
-			// If query fails, continue querying backwards
+			// If query fails, log and continue querying backwards
+			fmt.Printf("FilterFinalizeBatch error in range [%d, %d]: %v\n", startBlock, endBlock, err)
 			if endBlock < blockRange {
 				break // Already queried to block 0, exit loop
 			}

84-164: TestBatchRestartInit depends on live local nodes and on-chain state — consider a build tag or skip guard.

This test will fail in CI or on any machine without localhost:9545 / localhost:8545 running. A testing.Short() skip or a build tag (e.g., //go:build integration) would prevent spurious CI failures.

🔧 Proposed fix (apply to each test function)
 func TestBatchRestartInit(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping integration test in short mode")
+	}
 	testDir := filepath.Join(t.TempDir(), "testleveldb")

260-265: Remove custom min function—use Go's built-in instead.

The project targets Go 1.24.0. Since Go 1.21, min is a built-in function. This custom definition shadows it and can be removed.

tx-submitter/services/rollup.go (2)

1262-1268: common.Big0 is a shared global — consider using new(big.Int) to avoid aliasing risk.

SignedSequencersBitmap holds a pointer to common.Big0. If any downstream code (ABI encoding, contract interaction) mutates this *big.Int in place, it would corrupt the shared constant. Using new(big.Int) is a low-cost defensive measure.

🔧 Proposed fix
 	sigData := bindings.IRollupBatchSignatureInput{
-		SignedSequencersBitmap: common.Big0,
+		SignedSequencersBitmap: new(big.Int),
 		SequencerSets:          batch.CurrentSequencerSetBytes,
 		Signature:              []byte("0x"),
 	}

868-876: Finalize fetches the next batch to get ParentBatchHeader — verify this matches contract expectations.

nextBatchIndex = target.Uint64() + 1 fetches the batch after the one being finalized, using its ParentBatchHeader (which is the header of the batch being finalized). This is a non-obvious pattern. If the next batch hasn't been assembled yet, finalization will fail with a confusing "batch not found" error. Consider adding a comment clarifying the intent, and logging the target vs. next index for easier debugging.

tx-submitter/batch/batch_cache.go (3)

88-93: Constructor panics on L2 client failure — prefer returning an error.

panic(err) at Line 92 crashes the entire process if any L2 client is temporarily unavailable during construction. Callers cannot handle this gracefully. Consider returning (*BatchCache, error) from NewBatchCache.


560-584: FetchAndCacheHeader fetches the same block twice — once in CalculateCapWithProposalBlock and again at Line 579.

The second fetch is needed because PackCurrentBlock calls ClearCurrent, discarding the header. If this method is on a hot path, consider caching the header before clearing, or returning it from PackCurrentBlock.


699-728: CheckBatchSizeReached uses a rough heuristic that may be unreliable.

Line 724 estimates compressed size as blockContextsSize / 2, assuming ~2x zstd compression ratio. This is a loose approximation — actual compression ratios vary widely depending on data entropy. The SealBatch method already computes and returns the actual reachedExpectedSize, which is more accurate. Consider storing that value in the sealed batch or the storage layer instead of re-estimating.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tx-submitter/services/rollup.go (1)

1262-1268: ⚠️ Potential issue | 🔴 Critical

Signature verification is hardcoded to empty values — confirm this is intentional and that the contract accepts zero signatures.

buildSignatureInput unconditionally sets SignedSequencersBitmap: common.Big0 and Signature: []byte("0x"). The contract's commitBatch and commitBatchWithProof methods accept these fields and store SignedSequencersBitmap in state, indicating signatures are validated on-chain. Either document why empty signatures are acceptable for this phase, or implement proper BLS signature collection to avoid transaction failures.

🤖 Fix all issues with AI agents
In `@tx-submitter/batch/batch_cache.go`:
- Around line 358-369: Rename the misspelled variables in getBatchBlockRange:
change preBatchStorge to preBatchStorage and batchStorge to batchStorage (and
update any references in the same function) so the calls to
bc.rollupContract.BatchDataStore(nil, preIndex) and
bc.rollupContract.BatchDataStore(nil, batchIndex) assign into correctly spelled
variables before returning their BlockNumber values.
- Around line 850-878: buildBlockContext currently truncates txsNum and l1MsgNum
to uint16 without checks; add an explicit guard to prevent silent wrapping by
clamping both txsNum and l1MsgNum to math.MaxUint16 (65535) before casting and
writing to blsBytes, and optionally emit a warning/log when a clamp occurs;
update the assignments that write numTxs (blsBytes[56:58]) and numL1Messages
(blsBytes[58:60]) to use the clamped values instead of directly casting the
incoming ints.
- Around line 1087-1101: In AssembleCurrentBatchHeader(), fix the typo in the
error message returned when endBlockNum < bc.currentBlockNumber by replacing
"rerog" with "reorg" so the message reads something like "has reorg, should
check block status" (update the fmt.Errorf call that constructs the string using
bc.currentBlockNumber and endBlockNum).
- Around line 246-250: The code calls rollupContract.BatchDataStore(nil, fi)
using the finalized index `fi` instead of the parent batch index, which can set
bc.lastPackedBlockHeight to the wrong value; change the fallback to call
rollupContract.BatchDataStore(nil, parentBatchIndex) (the index derived from
parentHeader.LastBlockNumber()) and propagate/log any error so
bc.lastPackedBlockHeight is set from the correct BatchDataStore entry; update
the block that currently references BatchDataStore(nil, fi) to use
parentBatchIndex and preserve the existing error handling around the call.
- Around line 84-93: The constructor currently panics on L2 connectivity
failure; change NewBatchCache to return (*BatchCache, error) (or existing type
plus error) instead of panicking, and replace the panic(err) after calling
ifL2Clients.BlockNumber(ctx) with a returned wrapped error (e.g., fmt.Errorf("L2
client health check failed: %w", err)); update callers of NewBatchCache to
handle the error. Keep the default isBatchUpgraded assignment as-is and ensure
the connectivity check uses iface.L2Clients.BlockNumber(ctx) to validate clients
but bubbles the error up rather than terminating the process.

In `@tx-submitter/services/rollup.go`:
- Line 125: NewRollup currently calls batch.NewBatchCache(...) which may panic;
update NewRollup to call batch.NewBatchCache and handle the returned error
instead of assuming success: capture (bc, err) := batch.NewBatchCache(...), if
err != nil return nil, fmt.Errorf("creating batch cache: %w", err) (or propagate
appropriately), and assign bc to the Rollup.batchCache field; adjust NewRollup
signature to return (/*Rollup type*/, error) if not already so the error can be
propagated.
- Around line 718-734: handleDiscardedTx is calling r.pendingTxs.Add(replacedTx)
even though createReplacementTransferTx (which calls SendTx) already adds the tx
via SendTx's post-send hook; remove the duplicate add to avoid resetting
SendTime/QueryTimes — either delete the r.pendingTxs.Add(replacedTx) call in
handleDiscardedTx (and similar duplicate adds after ReSubmitTx/CancelTx) or
guard it with an existence check (only call Add if
pendingTxs.Contains(replacedTx.Hash()) is false) so SendTx remains the single
place that registers new pending transactions.
- Line 809: After successfully finalizing a batch, remove its on-disk data as
well as the in-memory entry: in the same place where
r.batchCache.Delete(batchIndex) is called, also invoke
r.batchStorage.DeleteSealedBatch(batchIndex) (handle/propagate any returned
error as appropriate) so sealed batch entries are removed from LevelDB and do
not accumulate on disk.
🧹 Nitpick comments (6)
tx-submitter/batch/batch_cache.go (5)

563-583: FetchAndCacheHeader fetches the same block twice.

CalculateCapWithProposalBlock (Line 565) already fetches the block from L2, but after packing, FetchAndCacheHeader re-fetches the same block at Line 579 just to return the header. Consider having CalculateCapWithProposalBlock or PackCurrentBlock stash the header so this extra RPC call is avoided.


706-728: CheckBatchSizeReached uses an unreliable heuristic.

The method estimates compressed size as blockContextsSize / 2, which is a rough approximation. Block contexts are structured data (not the actual transaction payload), so dividing by 2 doesn't reflect zstd compression of the real payload. SealBatch already computes and returns reachedExpectedSize — consider persisting that boolean in the sealed batch instead.


932-1009: Duplicate block fetch in assembleUnFinalizeBatchHeaderFromL2Blocks.

Each iteration fetches the same block twice — once inside CalculateCapWithProposalBlock (Line 958 → internally Line 457) and again at Line 964. This doubles the L2 RPC calls. Consider refactoring CalculateCapWithProposalBlock to accept an already-fetched block, or return the block it fetched.


1087-1180: AssembleCurrentBatchHeader duplicates ~90% of assembleUnFinalizeBatchHeaderFromL2Blocks logic.

Both methods share the same loop structure: iterate blocks, call CalculateCapWithProposalBlock, check exceeded/interval/timeout, seal, and pack. The only differences are how start/end block numbers are determined and how sealing is performed (with or without batch hash verification). Extract a shared method to reduce duplication and divergence risk.


122-157: Init() re-uses err from headerBytes.LastBlockNumber() in an inner scope, shadowing it.

On Line 143, err from LastBlockNumber() is checked and if non-nil, a new err is declared in the inner if block (Line 144), shadowing the outer err. While Go allows this, it makes it easy to miss that the original LastBlockNumber error is effectively swallowed — the fallback silently replaces it. Consider logging the original error for observability.

tx-submitter/services/rollup.go (1)

868-876: Add clarifying log or documentation: finalization requires the next batch to be committed.

The code correctly fetches batch at target + 1 because batch(target+1).ParentBatchHeader contains the header of batch target, which is what the contract's finalizeBatch function expects (verified via Solidity contract line 595: it validates finalizedStateRoots[_batchIndex - 1] against the batch header's previous state hash).

However, when the next batch isn't yet in the cache (line 873-876), the function returns nil with only a warn-level log. This behavior is correct by design—finalization necessarily waits for the next batch to be committed—but it's worth clarifying this constraint in the code. The current log at line 878 ("next batch is nil, wait next batch header to finalize") already hints at the reason; consider elevating this to Info level or adding an inline comment to make the dependency explicit for operators.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tx-submitter/services/rollup.go (1)

1262-1268: ⚠️ Potential issue | 🔴 Critical

Fix Signature: []byte("0x") — it contains invalid data for the signature field.

buildSignatureInput hardcodes the signature to []byte("0x"), which represents the literal ASCII string "0x" (2 bytes: [48, 120]). This is neither an empty byte slice nor valid signature data, and will likely cause contract validation failures when the calldata is packed.

Additionally, confirm that SignedSequencersBitmap: common.Big0 is intentional, as the prior staker bitmap querying logic has been removed entirely. Update the function to either:

  • Use []byte{} for the signature if no signature is required, or
  • Implement proper signature generation logic if a real signature is expected
🤖 Fix all issues with AI agents
In `@tx-submitter/batch/batch_cache.go`:
- Around line 985-1001: The code calls bc.GetSealedBatch(batchIndex) and ignores
the found boolean, then dereferences batch (batch.LastBlockNumber) which can
nil-panic; change the call site in SealBatchAndCheck/Seal path (where batch, _
:= bc.GetSealedBatch(batchIndex)) to check the returned found flag and return an
error if not found (include context like batchIndex and batchHash), only then
use batch.LastBlockNumber to compute startBlockNum and proceed to fetch
startBlock via bc.l2Clients.BlockByNumber; apply the identical guard/fix in
AssembleCurrentBatchHeader at the similar GetSealedBatch usage around line 1164
to avoid the same nil dereference.
- Around line 1057-1067: BatchCache.Delete currently only removes the entry from
the in-memory sealedBatches map; update it to also remove the persisted copy by
calling bc.batchStorage.DeleteSealedBatch(batchIndex) when the entry exists.
Keep the mutex protection around the in-memory delete, then invoke
bc.batchStorage.DeleteSealedBatch(batchIndex) (guarded by nil-check on
bc.batchStorage), and on error log via the cache's logger (e.g., bc.logger or
existing logging facility) so callers in rollup.go no longer leave stale LevelDB
data; still return the boolean indicating whether the in-memory entry existed.
- Around line 200-207: Accessing indices[0] before checking length is unsafe; in
the code that computes maxIndex from indices (variable names: indices,
maxIndex), first guard that len(indices) > 0 and only then assign maxIndex =
indices[0], otherwise handle the empty-case (e.g., return, skip computation, or
set to a sentinel) so you never index into an empty slice; adjust the loop
accordingly so the first element is used only after the length check.

In `@tx-submitter/batch/batch_query.go`:
- Around line 31-53: The loop can underflow when startBlock == 0 because
endBlock = startBlock - 1 will wrap; in the block where FilterFinalizeBatch
returns an error (the error handling after calling
bc.rollupContract.FilterFinalizeBatch), guard against underflow by checking if
startBlock == 0 and if so break/exit the loop (or set endBlock = 0 and break)
instead of performing startBlock - 1; update the error branch that currently
assigns endBlock = startBlock - 1 to handle the startBlock==0 case to safely
terminate the loop and avoid wrapping.
🧹 Nitpick comments (6)
tx-submitter/types/l2Caller.go (1)

74-87: TOCTOU window between SequencerSetVerifyHash and GetSequencerSetBytes calls.

The hash (line 75) and setBytes (line 79) are fetched in two separate RPC calls. If the sequencer set rotates between these calls, the verification at line 83 will fail. While the error is descriptive and calling code likely retries, consider using a single callOpts with a pinned block number to ensure both reads see the same state:

Suggested approach
 func (c *L2Caller) GetSequencerSetBytes(opts *bind.CallOpts) ([]byte, common.Hash, error) {
+	// Ensure both reads target the same block to avoid TOCTOU
+	if opts.BlockNumber == nil {
+		// Pin to latest known block if not already specified
+	}
 	hash, err := c.sequencerContract.SequencerSetVerifyHash(opts)

The callers in batch_cache.go do set callOpts.BlockNumber before calling this, so in practice this is likely safe. But if this method is ever called with opts.BlockNumber == nil, the two reads could hit different blocks.

tx-submitter/batch/batch_query.go (1)

57-83: Silent error swallowing makes debugging difficult.

Lines 64-66, 70-72, and 76-78 all continue on error without any logging. For a backward scan that might iterate many events, this makes it nearly impossible to diagnose why the function eventually fails to find a batch. Consider adding log.Debug or log.Warn for these cases.

tx-submitter/services/rollup.go (1)

868-876: Variable batch shadows the imported batch package.

Line 870 declares batch, ok := r.batchCache.Get(nextBatchIndex), which shadows the batch package imported on line 29. While functionally OK in this scope, renaming to e.g. nextBatch would avoid confusion and prevent issues if the batch package is referenced later in this function.

Proposed fix
-	batch, ok := r.batchCache.Get(nextBatchIndex)
-	if !ok {
-		log.Warn("get next batch by index failed, batch not found",
+	nextBatch, ok := r.batchCache.Get(nextBatchIndex)
+	if !ok {
+		log.Warn("get next batch by index failed, batch not found",
 			"batch_index", nextBatchIndex,
 		)
 		return nil
 	}
-	if batch == nil {
+	if nextBatch == nil {
 		log.Info("next batch is nil,wait next batch header to finalize", "next_batch_index", nextBatchIndex)
 		return nil
 	}
 
 	// calldata
-	calldata, err := r.abi.Pack("finalizeBatch", []byte(batch.ParentBatchHeader))
+	calldata, err := r.abi.Pack("finalizeBatch", []byte(nextBatch.ParentBatchHeader))
tx-submitter/batch/batch_storage.go (2)

37-60: Batch data can become orphaned if index update fails.

Line 54-56: If updateBatchIndices fails, the batch data is stored in LevelDB but not tracked in the indices list, making it unreachable via LoadAllSealedBatches. The warning log is good, but this could accumulate orphaned data over time.

Consider either failing the entire operation (so it can be retried atomically) or adding a recovery mechanism that scans for orphaned keys.


160-166: Mixed string/binary key encoding works but may surprise during debugging.

encodeBatchKey concatenates a human-readable prefix "sealed_batch_" with a raw 8-byte big-endian uint64. The binary portion won't be readable in LevelDB inspection tools. This is a minor ergonomics concern — not a correctness issue.

tx-submitter/batch/batch_cache.go (1)

562-583: FetchAndCacheHeader fetches the same block twice from L2.

The block is first fetched inside CalculateCapWithProposalBlock (line 446), then fetched again at line 578 after PackCurrentBlock clears the current state. If this backward-compat wrapper sees ongoing use, consider caching the header in the current-block state before clearing.

@FletcherMan FletcherMan linked an issue Feb 9, 2026 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable the submitter to independently assemble and submit batches

2 participants